Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build modification aimed to fix multiple instances of three #563

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

juanRabaa
Copy link

⚠️ All PRs have to be done versus 'dev' branch, so be aware of that, or we'll close your issue ⚠️

What kind of change does this PR introduce?

Can it be referenced to an Issue? If so what is the issue # ?
#504

How can we test it?
Run build and import three.js/build/ar-threex-location-only.js. I made a package of this fork to test this out and seems to work fine.

Summary

  • Changing threex-location-only output mode to module to fix multiple instances of threejs error when importing both three and the threex-location-only
  • Also modified build and build:dev scripts to use webpack instead of ./node_modules/.bin/webpack. It was producing an error in my machine and I guess others. It should work in every machine with just webpack.

Does this PR introduce a breaking change?

This will limit the threex-location-only build to be used only as a module. I think, if needed, more than one build can be made, one with the previous configuration UMD . I don't expect this to be merged as is, there probably is a better solution.

Please TEST your PR before proposing it. Specify here what device you have used for tests, version of OS and version of Browser

OS: Windows 10
Browser: Chrome V.116.0.5845.141

instances of threejs error when importing both `three` and the `threex-location-only`

- Also modified `build` and `build:dev` scripts to use `webpack` instead of
`./node_modules/.bin/webpack`. It was producing an error in my machine.
@nickw1
Copy link
Collaborator

nickw1 commented Dec 26, 2023

@juanRabaa thanks, will give this a go. @kalwalt do you also want to review this?

@nickw1
Copy link
Collaborator

nickw1 commented Dec 26, 2023

(sorry for late reply by the way, it's been a while since I've had the chance to look at AR.js).

@kalwalt
Copy link
Member

kalwalt commented Dec 27, 2023

@juanRabaa thanks, will give this a go. @kalwalt do you also want to review this?

(sorry for late reply by the way, it's been a while since I've had the chance to look at AR.js).

I hope to have some spare time after the 7 january, it was on my radar but i haven't had the time...

@nickw1
Copy link
Collaborator

nickw1 commented Sep 12, 2024

@juanRabaa finally got round to looking at your PR, many apologies for the delay.

I still get the "multiple instances" warning in my test code, which is tutorial 1 from the docs: https://ar-js-org.github.io/AR.js-Docs/location-based-three/ modified to use your build, e.g

import * as THREE from 'three';
import * as THREEx from '../juanRabaa/AR.js/three.js/build/ar-threex-location-only.js';

Does this work for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants